Simplify decomposition of controlled eigengates with global phase#7291
Closed
codrut3 wants to merge 1 commit intoquantumlib:mainfrom
Closed
Simplify decomposition of controlled eigengates with global phase#7291codrut3 wants to merge 1 commit intoquantumlib:mainfrom
codrut3 wants to merge 1 commit intoquantumlib:mainfrom
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7291 +/- ##
==========================================
+ Coverage 98.64% 98.66% +0.01%
==========================================
Files 1106 1106
Lines 95985 96144 +159
==========================================
+ Hits 94688 94864 +176
+ Misses 1297 1280 -17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
daxfohl
reviewed
Apr 21, 2025
| control_qubits = list(qubits[: self.num_controls()]) | ||
| # If the subgate is an EigenGate with non-zero phase, try to decompose it | ||
| # into a phase-free gate and a global phase gate. | ||
| if isinstance(self.sub_gate, eigen_gate.EigenGate) and self.sub_gate.global_shift != 0: |
Collaborator
There was a problem hiding this comment.
Is there any way you can get rid of this condition? Like try decomposing the subgate first, then only enter the branch if the subgate has decomposed? A big goal is to reduce the amount of type checking needed here.
If this works, then the duplicate function call at the end of this function can be removed, and the function can just return NotImplemented if it gets to that point.
daxfohl
reviewed
Apr 21, 2025
| # their own _decompose_ method. | ||
| self_without_phase._global_shift = 0 | ||
| global_phase = 1j ** (2 * self.global_shift * self.exponent) | ||
| return [self_without_phase.on(*qubits), global_phase_op.GlobalPhaseGate(global_phase)()] |
Collaborator
There was a problem hiding this comment.
Maybe check that the remaining phase isn't zero, and drop it if it is. This could be the case if shift==0.5 and exponent==4 for instance. The decomposition would factor out the shift, leaving, say, X**4 and an identity phase gate that there's no reason to keep.
daxfohl
reviewed
Apr 21, 2025
| assert cirq.decompose(op) == [ | ||
| (cirq.Y(q0) ** 0.5).with_classical_controls('a'), | ||
| cirq.XPowGate(exponent=1.0, global_shift=-0.25).on(q0).with_classical_controls('a'), | ||
| cirq.X(q0).with_classical_controls('a'), |
Collaborator
There was a problem hiding this comment.
Yeah, so here's where we probably need to have an opt in flag on the decompose context object. Having the basic gates suddenly decompose to multiple gates by default probably would be too likely to break something. While generally it seems that changing details around complex decompositions have been approved, this change seems a little too fundamental.
Collaborator
|
This seems to have gone stale, so I went ahead and posted a PR to do this while managing the backwards compatibility issues (#7383 if you want to take a look). I think this PR can be closed now. |
Contributor
Author
|
Sorry, I was working on another PR and I was planning to return to this afterwards. |
This was referenced Jun 1, 2025
Merged
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jun 2, 2025
This fixes a minor issue I found while working on #7291 The test case names don't match the method being tested. This is confusing because there is another method called `merge_single_qubit_moments_to_phxz`, but it's not the one tested here.
Contributor
Author
|
I extracted parts of this in other PRs. I'll go ahead and close it. Please review #7383 instead. |
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jun 17, 2025
…7405) This addresses an issue I found while working on #7291 Currently `merge_single_qubit_moments_to_phxz` will not merge moments that have a global phase, meaning that moments that could theoretically be merged are not. I updated it so that global phase is also supported. I added two tests to show the issue. Perhaps this situation is not common right now, but if #7383 is committed and the flag is used, it may become more common. @daxfohl Note that I'm not merging the global phase operations in a single one, because I'm thinking some phases could be parametrized; should I attempt to do this?
BichengYing
pushed a commit
to BichengYing/Cirq
that referenced
this pull request
Jun 20, 2025
This fixes a minor issue I found while working on quantumlib#7291 The test case names don't match the method being tested. This is confusing because there is another method called `merge_single_qubit_moments_to_phxz`, but it's not the one tested here.
BichengYing
pushed a commit
to BichengYing/Cirq
that referenced
this pull request
Jun 20, 2025
…uantumlib#7405) This addresses an issue I found while working on quantumlib#7291 Currently `merge_single_qubit_moments_to_phxz` will not merge moments that have a global phase, meaning that moments that could theoretically be merged are not. I updated it so that global phase is also supported. I added two tests to show the issue. Perhaps this situation is not common right now, but if quantumlib#7383 is committed and the flag is used, it may become more common. @daxfohl Note that I'm not merging the global phase operations in a single one, because I'm thinking some phases could be parametrized; should I attempt to do this?
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jun 23, 2025
This fixes a small issue that I found while working on #7291 TaggedOperation `_decompose_` calls `protocols.decompose_once` (see [here](https://github.com/quantumlib/Cirq/blob/e27d883d20d4acea97bd96d795170ae984b3dba9/cirq-core/cirq/ops/raw_types.py#L837)). So it must be compared against `cirq.decompose_once`. --------- Co-authored-by: Pavol Juhas <juhas@google.com>
BichengYing
pushed a commit
to BichengYing/Cirq
that referenced
this pull request
Jun 25, 2025
This fixes a small issue that I found while working on quantumlib#7291 TaggedOperation `_decompose_` calls `protocols.decompose_once` (see [here](https://github.com/quantumlib/Cirq/blob/e27d883d20d4acea97bd96d795170ae984b3dba9/cirq-core/cirq/ops/raw_types.py#L837)). So it must be compared against `cirq.decompose_once`. --------- Co-authored-by: Pavol Juhas <juhas@google.com>
ddddddanni
pushed a commit
to ddddddanni/Cirq
that referenced
this pull request
Jul 15, 2025
…uantumlib#7405) This addresses an issue I found while working on quantumlib#7291 Currently `merge_single_qubit_moments_to_phxz` will not merge moments that have a global phase, meaning that moments that could theoretically be merged are not. I updated it so that global phase is also supported. I added two tests to show the issue. Perhaps this situation is not common right now, but if quantumlib#7383 is committed and the flag is used, it may become more common. @daxfohl Note that I'm not merging the global phase operations in a single one, because I'm thinking some phases could be parametrized; should I attempt to do this?
ddddddanni
pushed a commit
to ddddddanni/Cirq
that referenced
this pull request
Jul 15, 2025
This fixes a small issue that I found while working on quantumlib#7291 TaggedOperation `_decompose_` calls `protocols.decompose_once` (see [here](https://github.com/quantumlib/Cirq/blob/e27d883d20d4acea97bd96d795170ae984b3dba9/cirq-core/cirq/ops/raw_types.py#L837)). So it must be compared against `cirq.decompose_once`. --------- Co-authored-by: Pavol Juhas <juhas@google.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7238 following the recommendations in the issue description: I added a
_decompose_method toEigenGatethat extracts global phase, and updatedControlledGate._decompose_with_context_to try to extract the global phase first.This PR may be undesirable because gates that fix
global_shiftneed to implement their own_decompose_method. I did this forRx,RyandRz, but I don't have a nice solution for a not-yet implemented gate that derives fromEigenGateand fixesglobal_shift: any such gate must have a_decompose_method. See the discussion in #7238.While implementing this change I found two issues:
assert [*tagged_h._decompose_()] == cirq.decompose_once(h).tagged_h._decompose_callsdecompose_once, so it must be compared withcirq.decompose_once, not withcirq.decompose. The difference became visible when a second step in the decomposition extracted the global phase.merge_single_qubit_moments_to_phxzdoesn't handle well global phase. A global phase gate gets assigned to the first moment, and in the current implementation this makes it unmergeable with the next moment. I found this while examining a test failure. I fixed this, and created a test case to show the problem.It may make sense to address these issues in separate PRs, independent of this one. Let me know!